Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ARCO-283): remove freecache, combine methods with cache implementation #643

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

pawellewandowski98
Copy link
Collaborator

Description of Changes

Provide a brief description of the changes you've made.

Linked Issues / Tickets

Reference any related issues or tickets, e.g. "Closes #123".

Testing Procedure

Describe the tests you've added or any testing steps you've taken.

  • I have added new unit tests
  • All tests pass locally
  • I have tested manually in my local environment

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have updated CHANGELOG.md with my changes

@pawellewandowski98 pawellewandowski98 self-assigned this Nov 13, 2024
@arkadiuszos4chain
Copy link
Collaborator

I would introduce a few changes (since we already virtualize the cache engine):

  1. implement a simple InMemoryCacheStore
  2. rename config.InternalCache to config.InMemory
  3. use InMemoryCacheStore as the default in the processor. This will simplify the code by reducing the number of if statements.

Bonus question: are you sure it's good to store the entire map in the cache instead of querying and saving individual records?

Copy link

sonarcloud bot commented Nov 14, 2024

if len(statusUpdate.CompetingTxs) > 0 {
statusUpdate.CompetingTxs = mergeUnique(statusUpdate.CompetingTxs, foundStatusUpdate.CompetingTxs)
}
err = p.cacheStore.Set(&CacheStatusUpdateHash, status.Hash.String(), bytes, processStatusUpdatesIntervalDefault)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to create a TTL that's equal to UpdateInterval, because there may be some hiccups with metamorph service, in which case we can loose data.

Also, this TTL is doing nothing when we're giving a hash to the method (see my next comment)

I would choose either:

  1. A very long const TTL in order not to clear records that for some reason were never queried.
  2. No TTL at all and periodical clearing cache (what I think we have currently already implemented with clearCache on every update.

Comment on lines +45 to +53
// Set stores a value with a TTL for a specific hash (if given).
func (r *RedisStore) Set(hash *string, key string, value []byte, ttl time.Duration) error {
var err error

if hash == nil {
err = r.client.Set(r.ctx, key, value, ttl).Err()
} else {
err = r.client.HSet(r.ctx, *hash, key, value).Err()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're only passing in the ttl when we're using the regular Set, otherwise, with HSet there's not TTL

Comment on lines +17 to +19
// Get retrieves a value by key and hash (if given)
func (s *MemoryStore) Get(hash *string, key string) ([]byte, error) {
if hash != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hash is optional, as I understand, but because of that, in every method we have to check if hash != nil. Also, currently, we're always using a hash and never access the general store.

What do you think about defining a list of allowedHashSets (e.g. in cache.go) with hashes of sets that we can use and only allow to use those HSets, instead of allowing using the general store? Or even better - just define those allowable sets in an Enum, which we can use everywhere in code.

Then, you could also check if given hash exists in allowedHashSets (or enum) and if not - return an ErrNotAllowedSet or something like that. You wouldn't have to retrieve values in a different way depending on whether the hash exists or not and it would not have to be a pointer.

Comment on lines +25 to +34
// Get retrieves a value by key and hash (if given).
func (r *RedisStore) Get(hash *string, key string) ([]byte, error) {
var result string
var err error

if hash == nil {
result, err = r.client.Get(r.ctx, key).Result()
} else {
result, err = r.client.HGet(r.ctx, *hash, key).Result()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we're only saving and returning a []byte type from the store. One idea for improvement would be to make this store generic and use jsons. Example:

type RedisStore struct {
	// ...
}

func (r *RedisStore) Get[T any](key string) (T, error) {
	val, err := r.client.Get(r.ctx, key)
	if err != nil {
		return result, err
	}

	// Assume Redis values are stored as JSON strings
	var result T
	err = json.Unmarshal([]byte(val), &result)
	if err != nil {
		return result, ErrFailedToParseValue
	}

	return result, nil
}

func (r *RedisStore) Set[T any](key string, value T) error {
	// Marshal the value into JSON
	data, err := json.Marshal(value)
	if err != nil {
		return ErrFailedToMarshall
	}

	err = r.client.Set(r.ctx, key, string(data))
	if err != nil {
		return ErrCacheFailedToSet
	}

	return nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm going too far and this is not required, or maybe it's for another task, but I'm throwing the idea here.

WDYT? @boecklim @arkadiuszos4chain @pawellewandowski98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants